Cancel nukes when accepting alliance via radial menu #3155

deshack · 2026-02-08T09:02:21Z

Resolves #3154

Description:

#2716 introduced nuke cancellation logic on alliance acceptance via AllianceRequestReplyExecution. The radial menu action, though, calls AllianceRequestExecution instead, which accepts the alliance if a request has already been made by the other player.

This PR moves the nuke cancellation logic to GameImpl, hooking into the acceptAllianceRequest method, therefore accounting for every alliance acceptance, regardless of the specific action that brought to that.

Please complete the following:

I have added screenshots for all UI updates
I process any text displayed to the user through translateText() and I've added it to the en.json file
I have added relevant tests to the test directory
I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

deshack_82603

…eptance way Fixes #3154

coderabbitai · 2026-02-08T09:02:57Z

Walkthrough

Moved nuke-cancellation from the old alliance-reply execution into core game acceptance logic; replaced the reply intent/execution with a reject flow and added an explicit AllianceRejectExecution. Tests and client wiring updated to use request/counter-request or explicit reject flows.

Changes

Cohort / File(s)	Summary
Alliance reply removal `src/core/execution/alliance/AllianceRequestReplyExecution.ts`	Deleted the reply execution implementation and removed its private `cancelNukesBetweenAlliedPlayers` method and related imports.
Game-level nuke cancellation `src/core/game/GameImpl.ts`	Added nuke-scanning/cancellation during alliance acceptance (cancels active AtomBomb/HydrogenBombs between newly allied players and emits messages).
New reject execution `src/core/execution/alliance/AllianceRejectExecution.ts`	Added `AllianceRejectExecution` to handle explicit rejection of alliance requests (validates requester, clears outgoing request, deactivates).
Request execution changes `src/core/execution/alliance/AllianceRequestExecution.ts`	Added exported `cancelNukesBetweenAlliedPlayers` helper (scans/deletes nukes and emits messages); adjusted imports to include MessageType and UnitType.
Intent/schema updates `src/core/Schemas.ts`	Replaced `AllianceRequestReplyIntent` schema/type with `AllianceRejectIntent` (type `"allianceReject"`; only `requestor: ID`) and updated IntentSchema union.
Execution routing `src/core/execution/ExecutionManager.ts`	Removed handling for `allianceRequestReply`; added handling for `allianceReject` and import of `AllianceRejectExecution`.
Client transport & UI wiring `src/client/Transport.ts`, `src/client/graphics/layers/EventsDisplay.ts`	Replaced `SendAllianceReplyIntentEvent` with `SendAllianceRejectIntentEvent`; EventsDisplay now uses `SendAllianceRequestIntentEvent` for accepts and `SendAllianceRejectIntentEvent` for rejects; Transport subscribes/emits the new event shape.
Tests updated `tests/Alliance.test.ts`, `tests/core/game/GameImpl.test.ts`	Replaced usages of the deleted reply execution with either a counter `AllianceRequestExecution` to form alliances or `AllianceRejectExecution` to reject; adjusted mocks and assertions accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🤝 A request, a counter, a tidy reject,
Mid-air bombs now quietly deflect.
Menus or clicks — the flow's now clean,
Alliances settle, nukes unseen. ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR includes refactoring of alliance request/reply/reject flows with test updates, which while related to the core fix, goes beyond the minimal scope needed to resolve `#3154`.	Consider whether refactoring AllianceRequestReplyExecution to AllianceRejectExecution is necessary for fixing nuke cancellation, or if it should be a separate PR to keep changes focused.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: moving nuke cancellation logic to handle alliance acceptance via radial menu.
Description check	✅ Passed	The description clearly explains the bug fix, references the linked issue `#3154`, and describes the solution of moving nuke cancellation logic to GameImpl.
Linked Issues check	✅ Passed	The PR addresses issue `#3154` by ensuring nuke cancellation happens for all alliance acceptance paths, including radial menu acceptance, by hooking into acceptAllianceRequest in GameImpl.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

src/client/graphics/layers/EventsDisplay.ts (1)
460-463: Argument order is correct but reads confusing — consider a clarifying comment.

SendAllianceRequestIntentEvent(recipient, requestor) — here recipient is the current player (who becomes the new requestor of the counter-request) and requestor is the original requestor (who becomes the new recipient). The semantics are correct: accepting = sending a counter alliance request.

A short inline comment would help future readers understand this is intentional, since the variable names appear swapped relative to the constructor parameters.
💡 Suggested clarifying comment
          action: () =>
            this.eventBus.emit(
-              new SendAllianceRequestIntentEvent(recipient, requestor),
+              // Accept by sending a counter-request: recipient (me) requests alliance with the original requestor
+              new SendAllianceRequestIntentEvent(recipient, requestor),
            ),
src/core/game/GameImpl.ts (1)

3-3: GameImpl imports from the execution layer — consider moving the helper.

cancelNukesBetweenAlliedPlayers is a pure game-logic utility (iterates units, checks ownership, deletes). Placing it in AllianceRequestExecution.ts (an execution-layer file) and importing it into GameImpl (core game layer) inverts the typical dependency direction (execution → game, not game → execution).

Consider moving the function to a shared location (e.g., alongside GameImpl.ts or a dedicated alliance utility file under src/core/game/) so the dependency flows cleanly downward.

tests/AllianceDonation.test.ts (1)

42-95: Duplicate test cases after the flow change.

The "by reply" and "by mutual request" tests now use the exact same code (both do counter-request via AllianceRequestExecution). Since the old reply flow no longer exists, these pairs are duplicates:

"Can donate gold after alliance formed by reply" (line 42) = "Can donate gold after alliance formed by mutual request" (line 78)

"Can donate troops after alliance formed by reply" (line 61) = "Can donate troops after alliance formed by mutual request" (line 97)

Consider merging each pair into a single test or removing the "by reply" variants — they no longer test a distinct path.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

evanpelle · 2026-02-10T00:23:20Z

src/core/execution/alliance/AllianceRejectExecution.ts

Would it be possible to just remove this execution? And then events display and radial menu can just send AllianceRequest? Then all alliance logic would be in a single file, and we can move cancelNukesBetweenAlliedPlayers there.

I centralised acceptance logic in AllianceRequestExecution and created a new AllianceRejectExecution to handle rejections, because:

semantics: you'll never look for the rejection path inside Alliance Request Execution

SRP: adding a new parameter to AllianceRequestExecution to branch out its logic and handle rejections felt really wrong to me

evanpelle · 2026-02-10T00:24:06Z

src/core/game/GameImpl.ts

    });
  }

+  private cancelNukesBetweenAlliedPlayers(p1: Player, p2: Player): void {


GameImpl should ideally be just a container to hold game state and not have much complex logic.

Moved to AllianceRequestExecution

evanpelle · 2026-02-12T19:06:28Z

src/core/execution/alliance/AllianceRejectExecution.ts

+    const requestor = mg.player(this.requestorID);
+
+    if (requestor.isFriendly(this.recipient)) {
+      console.warn("already allied");


here and on line 28 can you log something like:

console.warn("[AllianceRejectExecution] Player ${this.requestorID} cannot reject alliance with ${this.recipient.id}, already allied"

evanpelle · 2026-02-12T19:10:07Z

src/core/game/GameImpl.ts

+    // Cancel incoming nukes between players
+    cancelNukesBetweenAlliedPlayers(this, requestor, recipient);
+


this also adds a lot of logic into GameImpl, gameimpl should ideally just store & update state. Can this be removed?

Same with line 337 & 338

Move cancel nukes logic to GameImpl to account for every alliance acc…

6370ed6

…eptance way Fixes #3154

deshack requested a review from a team as a code owner February 8, 2026 09:02

github-project-automation bot added this to OpenFront Release Management Feb 8, 2026

github-project-automation bot moved this to Triage in OpenFront Release Management Feb 8, 2026

github-actions bot assigned deshack Feb 8, 2026

coderabbitai bot previously approved these changes Feb 8, 2026

View reviewed changes

evanpelle reviewed Feb 10, 2026

View reviewed changes

Merge branch 'main' into fix-cancel-nukes

a08973d

openfrontio bot temporarily deployed to staging February 12, 2026 08:19 Inactive

Fix review comments

26b2d8e

deshack dismissed coderabbitai[bot]’s stale review via 26b2d8e February 12, 2026 09:03

openfrontio bot temporarily deployed to staging February 12, 2026 09:03 Inactive

coderabbitai bot approved these changes Feb 12, 2026

View reviewed changes

deshack requested a review from evanpelle February 12, 2026 09:19

evanpelle reviewed Feb 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel nukes when accepting alliance via radial menu #3155

Cancel nukes when accepting alliance via radial menu #3155

Uh oh!

deshack commented Feb 8, 2026

Uh oh!

coderabbitai bot commented Feb 8, 2026 •

edited

Loading

Uh oh!

evanpelle Feb 10, 2026

Uh oh!

deshack Feb 12, 2026

Uh oh!

evanpelle Feb 10, 2026

Uh oh!

deshack Feb 12, 2026 •

edited

Loading

Uh oh!

evanpelle Feb 12, 2026

Uh oh!

evanpelle Feb 12, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Cancel incoming nukes between players
		cancelNukesBetweenAlliedPlayers(this, requestor, recipient);

Cancel nukes when accepting alliance via radial menu #3155

Are you sure you want to change the base?

Cancel nukes when accepting alliance via radial menu #3155

Uh oh!

Conversation

deshack commented Feb 8, 2026

Description:

Please complete the following:

Please put your Discord username so you can be contacted if a bug or regression is found:

Uh oh!

coderabbitai bot commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

evanpelle Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

deshack Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

evanpelle Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

deshack Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanpelle Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

evanpelle Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 8, 2026 •

edited

Loading

deshack Feb 12, 2026 •

edited

Loading

evanpelle Feb 12, 2026 •

edited

Loading